-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Suppport for D2L files proxing using Via #4792
Conversation
d77f90f
to
a403f9d
Compare
e006c0a
to
533b08f
Compare
a403f9d
to
40f6193
Compare
# We don't need the data from this call. | ||
# We are only interested on the potential side effect of needing | ||
# a new access token and/or refreshing an existing one. | ||
_ = self._api.request( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a hack but the whole setup is (sending the header etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because we're going to be sending our access token to Via (in the new secret headers), and we therefore need to make sure that the access token is still valid first?
|
||
public_url = request.find_service(D2LAPIClient).public_url(course_id, file_id) | ||
|
||
access_token = request.find_service(name="oauth2_token").get().access_token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I might need to get this from a private method in the client but we can just use the oauth2 service 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested it myself, but this all looks totally sound to me.
elif document_url.startswith("d2l://"): | ||
self._config["api"]["viaUrl"] = { | ||
"authUrl": self._request.route_url(D2L.route.oauth2_authorize), | ||
"path": self._request.route_path( | ||
"d2l_api.courses.files.via_url", | ||
course_id=self._request.lti_params["context_id"], | ||
_query={"document_url": document_url}, | ||
), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, this is the config telling the frontend to make the API call to get the Via URL? 👍
config.add_route( | ||
"d2l_api.courses.files.via_url", "/api/d2l/courses/{course_id}/via_url" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the new Via URL API for the frontend to call go get D2L files Via URLs 👍
@@ -117,6 +117,32 @@ def course_table_of_contents(self, org_unit) -> List[dict]: | |||
) | |||
return D2LTableOfContentsSchema(response).parse().get("modules", []) | |||
|
|||
def public_url(self, org_unit, file_id) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Service method for getting a public URL for a D2L file. So far this is all very familiar 😄
To make sure the API authentication header is not expired we'll make a API | ||
request here so if it needs to be refreshed we follow the standard token refresh procedure. | ||
|
||
https://docs.valence.desire2learn.com/res/content.html#get--d2l-api-le-(version)-(orgUnitId)-content-topics-(topicId) | ||
https://docs.valence.desire2learn.com/res/content.html#get--d2l-api-le-(version)-(orgUnitId)-content-topics-(topicId)-file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is probably better in code comments next to the code that makes these requests. Otherwise it seems likely that if the code changes this docstring will get out of date.
# We don't need the data from this call. | ||
# We are only interested on the potential side effect of needing | ||
# a new access token and/or refreshing an existing one. | ||
_ = self._api.request( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because we're going to be sending our access token to Via (in the new secret headers), and we therefore need to make sure that the access token is still valid first?
@@ -8,7 +8,7 @@ | |||
from lms.validation.authentication import OAuthCallbackSchema | |||
|
|||
GROUPS_SCOPES = ("groups:*:*",) | |||
FILES_SCOPES = ("content:toc:read",) | |||
FILES_SCOPES = ("content:toc:read", "content:topics:read") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what will happen to people who've already granted us access tokens without the topics scope? Will they get prompted to re-authorize us? Maybe no real users are using this feature yet anyway?
40f6193
to
4a7872d
Compare
644350e
to
8294040
Compare
Needs
Testing
I've been using
.tox/dev/bin/pip install -e /home/marcos/hypo/h-vialib
for this.https://aunltd.brightspacedemo.com/d2l/le/content/6782/viewContent/2184/View
via.secret.headers